Logic to capture reservoir changes with replaceComponent PumpEventType #89
Conversation
There was a problem hiding this comment.
LGTM as this is just a function naming change. I do wonder about potential confusion that may come by equating rewind and reservoir change and the same thing. They are very similar, but not exactly the same. Obviously this is very pump specific, and the underlying event is putting new insulin in the pump, however that happens. Since the naming is being updated, should it be updated to something that would work for any potential pump (personally I like the idea of moving away from something happening to the reservoir and towards putting new insulin in the pump).
| return dataForRewind(for: userId, hostIdentifier: hostIdentifier, hostVersion: hostVersion) | ||
| return dataForReservoirChange(for: userId, hostIdentifier: hostIdentifier, hostVersion: hostVersion) | ||
| case .suspend: | ||
| return dataForSuspend(for: userId, hostIdentifier: hostIdentifier, hostVersion: hostVersion) | ||
| case .replaceComponent(componentType: .reservoir): | ||
| return dataForReservoirChange(for: userId, hostIdentifier: hostIdentifier, hostVersion: hostVersion) |
There was a problem hiding this comment.
[nit] semantically, rewind and reservoir change are not the same. Should the naming be updated to remove any confusion (maybe something like dataForReservoir or dataForInsulinReplacement... I'm not good at naming things)?
There was a problem hiding this comment.
Eventually, we want the backend to absorb all of the replaceComponent pump event types (reservoir, pump, and infusionSet) added:
https://github.com/tidepool-org/LoopKit/pull/536/files
Currently we only have the TReservoirChangeDeviceEventDatum utilized by the .rewind case (not sure if this is accurate or a manipulation of the backend datum to accommodate a Medtronic rewind event) to capture a reservoir change event. We now have a new case replaceComponent(componentType: .reservoir) that needs to be accommodated but would essentially be the same function as rewind. I like your naming dataForReservoir as it tracks with the TReservoirChangeDeviceEventDatum type being used, should we go with this naming?
There was a problem hiding this comment.
The function name is good as is, I believe; it should match the name of the model that it's creating on the back-end. We are mapping multiple pump event types to a single model on the backend, because Loop's model is a little more fine-grained.
https://tidepool.atlassian.net/jira/software/c/projects/COASTAL/boards/78?modal=detail&selectedIssue=LOOP-227
Related PR:
tidepool-org/LoopKit#538